Codecov Report

Merging #553 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
+ Coverage   99.28%   99.28%   +<.01%     
==========================================
  Files          89       89              
  Lines       10491    10498       +7     
  Branches      728      728              
==========================================
+ Hits        10416    10423       +7     
  Misses         58       58              
  Partials       17       17

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c17a38c...82be61e. Read the comment docs.

@smurfix Ouch, sorry this made you waste time :( Thanks for adding a test and providing a fix!

I'm late for review but I want to add a note: I think that cleanup line pertains in the abort function, rather than in a finally clause. The abort function is only called on cancellation, while the finally clause is in the success path. And hopefully wait_task_rescheduled() only raises on cancellation.

Umm, no, the finally clause is in the "in any case" path. The await will get an exception (most likely trio.Cancelled).
The proof is in the pudding, i.e. if you run the second patch without the first, the test will fail.
In fact I probably should use except BaseException instead of finally because int the success case the task is guaranteed to have been removed from the dict. That should be somewhat faster, given that .pop() is likely to raise+catch an internal KeyError.

@smurfix I meant that the finally sits in the way of the success path, because it's executed every time instead of just when it's needed. As you correctly say, this can make make the success case (slightly) slower.

What I suggest is:

def abort_fn(_):
    self._get_wait.pop(task, None)
    return _core.Abort.SUCCEEDED

value = await _core.wait_task_rescheduled(abort_fn)

OK, thanks for clarifying. I didn't do that on purpose because there may be other exceptions (KeyboardInterrupt, anybody?) which would trigger the same problem.

My point was stronger than that. No other exceptions may occur in a sane state :) KeyboardInterrupt cannot happen in that code, wait_task_rescheduled() on its own is not supposed to throw, and only CancelError can be sent by the task scheduler. This is not a fragile coincidence that can change without notice: it's like this by design. abort_func is the elected place for cleanup, and it was my mistake to forget about it (to be fair, an earlier version of this code used to do exactly the same, then I removed it in 7cb8e14).

@smurfix I see that @pquentin already reminded you about the don't-merge-your-own-PR guideline in chat. No harm done, but just want to mention it here too in case others watching get confused. As you can see, even a small and fundamentally correct fix like this still can have some fiddly bits to get right...

@sorcio is correct that abort_fn is the canonical place to do this kind of cleanup. There's even an issue open (#315) suggesting that it might be simpler to unconditionally call abort_fn when rescheduling, to reduce code duplication between the normal and cancellation wakeup paths...

Note also that get is decorated with enable_ki_protection, so the only way KeyboardInterrupt can be raised there is through the cancellation mechanism (i.e., KeyboardInterrupt can happen, but it's guaranteed to call abort_fn).

Also, in the test, that 0.01 second timeout is making me itch. I think it does deterministically work right now, but the only reason there's no race condition is because of some subtle details about how the body of get interacts with the scheduler... I think it would be clearer and more reliable to write it like:

async def test_553(autojump_clock):
    q = trio.Queue()
    with trio.move_on_after(10) as timeout_scope:
        await q.get()
    assert timeout_scope.cancelled_caught
    await q.put("Test for PR #553")

given that .pop() is likely to raise+catch an internal KeyError

Apparently not:

# Popping a key that's present
In [3]: %timeit d = {1: None}; d.pop(1, None)
187 ns ± 8.37 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

# Popping a key that's missing
In [4]: %timeit d = {1: None}; d.pop(2, None)
176 ns ± 3.32 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)